CFE-3653: Prototyped dnf_appstream custom promise type #117
Conversation
|
|
||
| def _validate_state(self, value): | ||
| if value not in ("enabled", "disabled", "installed", "removed"): | ||
| raise ValidationError("state attribute must be 'enabled', 'disabled', 'installed', or 'removed'") |
There was a problem hiding this comment.
| raise ValidationError("state attribute must be 'enabled', 'disabled', 'installed', or 'removed'") | |
| raise ValidationError("State attribute must be 'enabled', 'disabled', 'installed', or 'removed'") |
| self.add_attribute("stream", str, required=False) | ||
| self.add_attribute("profile", str, required=False) |
There was a problem hiding this comment.
Why not add validators here like for the state attribute?
| # Check the module state | ||
| if module.status == "enabled": | ||
| return "enabled" | ||
| elif module.status == "disabled": | ||
| return "disabled" | ||
| elif module.status == "installed": | ||
| return "installed" |
There was a problem hiding this comment.
This can be simplified a bit
| # Check the module state | |
| if module.status == "enabled": | |
| return "enabled" | |
| elif module.status == "disabled": | |
| return "disabled" | |
| elif module.status == "installed": | |
| return "installed" | |
| # Check the module state | |
| if module.status in ("enabled", "disabled", "installed"): | |
| return module.status |
| module_base.enable([module_spec]) | ||
| module_base.base.resolve() | ||
| module_base.base.do_transaction() | ||
| self.log_verbose(f"Module {module_spec} enabled successfully") |
There was a problem hiding this comment.
Repaired should print info log messages, I think 🤔
| else: | ||
| return self._remove_module(module_base, module_spec) | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
I would remove this try/except since it will hide all bugs. The Promise type already handles any unexpected exceptions here
Also try to keep only the code that throws exceptions inside the try block for readability. Also only catch the Exceptions that you expect. And not all possible ones with except Exception.
I wasn't looking at the ticket prior to implementation, likely missing things. Ticket: CFE-3653
- Improve state validation error message. - Move validation to attribute validators. - Simplify state checking logic. - Use info logging for repairs. - Remove redundant try/except blocks. - Add test and documentation files.
nickanderson
force-pushed
the
CFE-3635/master
branch
from
af7b8f4 to
568afb1
Compare
| "dependencies": [], | ||
| "test_command": "python3 test_dnf_appstream.py", | ||
| "version": "0.0.1" | ||
| } No newline at end of file |
| # If we get here, module is not found or not in the specified stream | ||
| return "removed" | ||
|
|
||
| except Exception as e: |
There was a problem hiding this comment.
What exceptions do you expect here? Maybe you can handle the ones you expect and not all exceptions? If you don't expect any exceptions, then please remove the try/catch here and other places :)
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| DnfAppStreamPromiseTypeModule().start() No newline at end of file |
There was a problem hiding this comment.
| DnfAppStreamPromiseTypeModule().start() | |
| DnfAppStreamPromiseTypeModule().start() | |
| from cfengine_module_library import ValidationError | ||
| except ImportError as e: | ||
| print(f"Import error: {e}") | ||
| print("Make sure cfengine_module_library.py is in the correct location") |
There was a problem hiding this comment.
This is misleading if it is dnf_appstream that fails to import
| # Test valid attributes | ||
| try: | ||
| module.validate_promise("nodejs", { | ||
| "state": "enabled", | ||
| "stream": "12" | ||
| }, {}) | ||
| print(" ✓ Valid attributes validation passed") | ||
| except Exception as e: | ||
| print(f" ✗ Valid attributes validation failed: {e}") |
There was a problem hiding this comment.
Consider using the pytest framework
2 participants
I wasn't looking at the ticket prior to implementation, likely missing things.
Ticket: CFE-3653